QA Summary

Test Coverage

Terraform Summary

Hint: If "Terraform Format & Style" failed, run terraform fmt -recursive from the terraform/ directory and commit the results.

Output

stdout:
Success! The configuration is valid.


-------------------------------------
stderr:

Pusher: @lsr-explore, Action: pull_request_target, Workflow: Continuous Integration

@TylerHendrickson thanks for checking this out, I missed this comment. Out of curiosity, do you have an example off the top of your head where we use an env var in that way? I was looking into how we use feature flags, but it seems that those are scoped to the client side?

@masimons re

Out of curiosity, do you have an example off the top of your head where we use an env var in that way? I was looking into how we use feature flags, but it seems that those are scoped to the client side?

The implementation details for #3060 provide some instructions for implementing an API-side feature flag using a SHARE_TERMINOLOGY_ENABLED environment variable (plus a client-side feature flag, which doesn't apply here). This was implemented by PR #3147.

Fundamentally, this sort of thing would entail the following:

• An env var default value for dev environments, defined in packages/server/.env.example.
• Configuring the value for Staging environments as a key/value pair in the api_container_environment input variable in terraform/staging.tfvars.
• Configuring the value for Production environments via the same input variable in terraform/prod.tfvars.

I'd recommend that an environment variable that provides a binary feature flag be evaluated as a string using strict equality, e.g. my_feature_is_enabled = process.env.MY_FEATURE_ENABLED === 'true'.

@masimons I really like how this implementation parameterizes the flag in src/db/index.js and allows the caller to decide whether to pull in the toggle from env var or something else. This is looking good overall!

I did notice in local testing that setting SHOW_FORECASTED_GRANTS=false in packages/server/.env doesn't seem to have the desired effect (I could still see forecasted grants in search results), although when the env var is altogether missing, forecasted grants are hidden like we'd expect.

I believe this is because the packages/server/db/index.js functions are expecting a true boolean value (or else undefined, so that the arg default of false in the function signature is used), but when a value is provided from process.env.SHOW_FORECASTED_GRANTS, it's a string rather than a boolean, and !"false" is evaluating to true. I think the simplest way to resolve this is to have callers explicitly check whether process.env.SHOW_FORECASTED_GRANTS === "true", which I provided in a comment suggestion.

@TylerHendrickson , @masimons - I've accepted Ty's changes - this should be ready for review

@lsr-explore Thanks for the updates. I think we need the strict === 'true' comparison for all usages of the environment variable, so I added a few additional comment suggestions to address those as well. I think that I covered all of them, but feel free to double check me on that 🙂

Sorry that I missed this @TylerHendrickson . I applied the changes and did another sanity check of the code. I noticed a couple additional places where the flag needed to be sent and added a commit.

I noticed that the db.getGrantsInterested didn't have this check. This would only be an issue if we needed to turn off grants after enabling it and user had clicked follow.

Do you think we need the check on getGrantsInterested?

I noticed that the db.getGrantsInterested didn't have this check. This would only be an issue if we needed to turn off grants after enabling it and user had clicked follow.

Do you think we need the check on getGrantsInterested?

That's a fair point - I don't know if that particular case came up when we were thinking about this